Skip to content

Add special function to match lifetimes#5320

Merged
chrchr-github merged 15 commits intocppcheck-opensource:mainfrom
pfultz2:match-lifetime
Aug 14, 2023
Merged

Add special function to match lifetimes#5320
chrchr-github merged 15 commits intocppcheck-opensource:mainfrom
pfultz2:match-lifetime

Conversation

@pfultz2
Copy link
Copy Markdown
Contributor

@pfultz2 pfultz2 commented Aug 12, 2023

This also removes the termination checking in valueFlowUninit as this causes a lot of FNs.

Comment thread test/testuninitvar.cpp Outdated
" int b = ab.b;\n"
"}");
ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: ab.b\n", errout.str());
TODO_ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: ab.b\n", "", errout.str());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the second forward overwrite values that were already set? Seems like MemberExpressionAnalyzer is doing the right thing here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The valueflow is correct for this case, the . in ab.b is a Uninit. For some reason, we dont warn in the checker. I am not sure why.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, its a valueflow issue, we are too aggressive at forwarding to the parent =.

Comment thread test/testuninitvar.cpp
" x.push_back(a);\n"
"}\n");
ASSERT_EQUALS("[test.cpp:9]: (error) Uninitialized variable: a.j\n", errout.str());
ASSERT_EQUALS("[test.cpp:9]: (error) Uninitialized variable: a\n", errout.str());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that ideally it's better to write a member that is not initialized. It's something explicit that the user can investigate carefully.

Does this PR mean the analysis will be better? Less false negatives or false positives?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that ideally it's better to write a member that is not initialized. It's something explicit that the user can investigate carefully.

We write the variables when it is partially initialized, but if all members are uninitialized we just write the struct.

Does this PR mean the analysis will be better?

Yes because we will track aliases better across all of our analyzers not just SubExpressionAnalyzer. This should lead to less FPs. And will help enable more FNs in the future as well.

@firewave
Copy link
Copy Markdown
Collaborator

The selfcheck failures shows two accessMoved on the same line but a different column.

@chrchr-github chrchr-github merged commit 52081ef into cppcheck-opensource:main Aug 14, 2023
@firewave
Copy link
Copy Markdown
Collaborator

The selfcheck failures shows two accessMoved on the same line but a different column.

Having the same warning twice for the same code looks very much like a bug to me. That's why I pointed it out.

On a side note you could have left the move and just stored the value beforehand.

@pfultz2 pfultz2 deleted the match-lifetime branch August 14, 2023 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants